Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve cache_size and cache_size_percent descriptions #696

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

daisukebe
Copy link
Contributor

Description

Added additional explanations to the cloud_storage_cache_size and cloud_storage_cache_size_percent descriptions in https://docs.redpanda.com/current/reference/properties/object-storage-properties

Review deadline: N/A

Adding @jcipar as a SME reviewer

Page previews

Checks

  • New feature
  • Content gap
  • Support Follow-up
  • Small fix (typos, links, copyedits, etc)

@daisukebe daisukebe requested a review from jcipar August 21, 2024 02:49
@daisukebe daisukebe requested a review from a team as a code owner August 21, 2024 02:49
Copy link

netlify bot commented Aug 21, 2024

Deploy Preview for redpanda-docs-preview ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit 25d3898
🔍 Latest deploy log https://app.netlify.com/sites/redpanda-docs-preview/deploys/670635d382ff1000087c0fa5
😎 Deploy Preview https://deploy-preview-696--redpanda-docs-preview.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@Deflaimun
Copy link
Contributor

Deflaimun commented Aug 21, 2024

Hi @daisukebe . Is there an equivalent PR to the source code? Asking because the properties are extracted from automation through the source code. If they're not, it's ok, as I can include as part of my PR to source.

@daisukebe
Copy link
Contributor Author

@Deflaimun , ah not really. Once we update the description in the code https://github.com/redpanda-data/redpanda/blob/dev/src/v/config/configuration.cc#L2407-L2427, will the doc be updated automatically?

@Deflaimun
Copy link
Contributor

Not 100% automated, as in a CI/CD process. But currently a manual process where we run https://github.com/Deflaimun/redpanda-property-extractor/blob/main/property_extractor.py
and
https://github.com/Deflaimun/redpanda-property-extractor/blob/main/json-to-asciidoc/webpage-gen.py

Based on the release tag.

However we did a number of improvements on the descriptions and I have to forward a PR to source updating them.
So for now, it's ok to update directly on docs, as I can forward the changes back into source later.

@daisukebe
Copy link
Contributor Author

Awesome, thank you @Deflaimun !

Copy link
Contributor

@kbatuigas kbatuigas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@daisukebe let me know how this suggestion sounds. cc @Deflaimun

@daisukebe
Copy link
Contributor Author

LGTM and committed. Thank you for making it clearer @kbatuigas !

@@ -85,7 +85,7 @@ AWS or GCS bucket that should be used to store data.

Max size of object storage cache.

If both this property and <<cloud_storage_cache_size_percent>> are set, Redpanda uses the minimum of the two.
If both this property and <<cloud_storage_cache_size_percent>> are set, Redpanda uses the minimum of the two. If one of them is set to 0, Redpanda uses the non-zero one. If they are both set to 0, the cache size is 0.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Before making this change, I'd want to see what happens if the cache size is actually 0. This makes it sound like we support a cache size of 0, but I imagine cloud storage just wont work below some minimal cache size.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

any resolution on this @jcipar @daisukebe ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given there's no guardrails on setting to zero from preventing, we should document like below?

If both this property and <<cloud_storage_cache_size_percent>> are set, Redpanda uses the minimum of the two. If one of them is set to 0, Redpanda uses the non-zero one. Redpanda strongly recommends you do not set both of them to zero.

@jcipar @WillemKauf ^

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jcipar and @WillemKauf Looks like this PR is stuck and awaiting your guidance here. Please respond asap. Thx.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @Feediver1, I just put out a PR that will update the behaviour here and (hopefully) make the update to the docs more straight-forward. We will now prevent the following invalid configurations:

  1. cloud_storage_cache_size and cloud_storage_cache_size_percent cannot both be 0
  2. cloud_storage_cache_size cannot be 0 while cloud_storage_cache_size_percent is nil (std::nullopt)

I would say something like:

If both this property and <<cloud_storage_cache_size_percent>> are set, Redpanda uses the minimum of the two. If one of them is set to 0, Redpanda uses the non-zero one. These properties cannot both be `0` simultaneously. 

Hope this helps.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@WillemKauf @jcipar - The description has now been updated to reflect the change in the code. Please could you take a look and approve.

Also, please provide guidance on what happens when both properties are set to 0 so that we can provide troubleshooting advice where necessary.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the user attempts to set both configs to 0 via rpk cluster config edit, the configuration will be rejected (not applied) and the error message Cannot set both cloud_storage_cache_size and cloud_storage_cache_size_pct to 0. will be provided.

@@ -601,7 +601,7 @@ Divide the object storage cache across the specified number of buckets. This onl

Maximum size of the cloud cache as a percentage of unreserved disk space (see config_ref:disk_reservation_percent,true,cluster-properties[]). The default value for this option is tuned for a shared disk configuration. Consider increasing the value if using a dedicated cache disk.

The property <<cloud_storage_cache_size,`cloud_storage_cache_size`>> controls the same limit expressed as a fixed number of bytes. If both `cloud_storage_cache_size` and `cloud_storage_cache_size_percent` are set, Redpanda uses the minimum of the two.
The property <<cloud_storage_cache_size>> controls the same limit expressed as a fixed number of bytes. If both `cloud_storage_cache_size` and `cloud_storage_cache_size_percent` are set, Redpanda uses the minimum of the two. If one of these properties is set to 0, Redpanda applies the non-zero value. If they are both set to 0, the cache size is 0.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems to contradict the text above, and how I understand the code. I think this should also read "These properties cannot both be 0."

@daisukebe
Copy link
Contributor Author

Apologies for not updating from me. I will gather them up this week. Thank you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants